feat: Sign In progress indicator and timeout COMPASS-10996 - #8391

I think it's confusing that while we have a method that should contain all attempt creation logic it starts to spill out of it, can we revisit this?

I would also maybe suggest to look at the startAttempt once more and consider if more of the logic here that handles cancellation / timeouts can be moved directly to the sign in handling action: both timeouting or cancelling throw a clear error that can be handled (and is already sort of handled) inside the sign in flow

I'm not sure if I follow what you mean here. The timeout and cancel functions need to exist as they'd be called when something happens in the background. However they're doing a few things today (clearing the timeout, clearing the AttemptStateMap, aborting, dispatching their event and tracking their telemetry event).

I could update the signIn catch do something like:

catch (err) {
      clearTimeout(getAttempt(currentAttemptId).timeoutId);
      AttemptStateMap.delete(currentAttemptId);

      if (!signal.aborted) {
        openToast('atlas-sign-in-error', {
          variant: 'important',
          title: 'Sign in failed',
          description: (err as Error).message,
        });
        dispatch({
          type: AtlasSignInActions.Error,
          error: (err as Error).message,
        });
      }
      
      reject(err);
}

Then both the timeoutSignIn and cancelSignIn would simply do something like

export const cancelSignIn = (reason?: any): AtlasSignInThunkAction<void> => {
  return (dispatch, getState, { track }) => {
    if (getState().currentAttemptId === null) {
      return;
    }
    getAttempt(getState().currentAttemptId).controller.abort(reason ?? 'Sign in canceled');
    dispatch({ type: AtlasSignInActions.Cancel });
    track('Atlas Sign In Canceled', {});
  };
};

Is that what you meant or was it something else?

Apologies for not being more detailed there, sometimes hard for me to measure into how much details to go into. What you have here in the suggested refactor is going into the right direction, yeah. What I'm suggesting is to take it even further and package timeout (and more of the cancel handling) completely inside the sign in (we need cancel to be separate because it can be triggered from outside, timeout is completely encapsulated to the flow). In pseudocode something close to this:

function signInAction() {
  try {
    dispatch('SignInStart')
    await Promise.race([
      doSignIn(),
      new Promise((_, reject) => {
        setTimeout(() => { attempt.controller.abort(new TimeoutError()); }, TIMEOUT_MS) 
      })
    ])
  } catch (err) {
    if (attempt.controller.signal.aborted) {
      if (attempt.controller.signal.reason === TimeoutError) {
        // do the timeout handling
        dispatch('SignInTimeout')
      } else {
        // do the cancelled handling
        dispatch('SignInCancel');
      }
    } else {
      // do the other error types handling
      dispatch('SignInError')
    }
  }
}

function cancelAction(reason) {
  return (dispatch, getState) {
    getAttempt(getState().attemptId)?.controller.abort(reason);
  }
}

I think that way you have as much of the sign in logic readable and understandable inside one method without the need of jumping through multiple actions to figure out what's going on. There's also a lot of similarities between how you handle timeout and generic cancel that this allows you to resolve clearly.

I'm okay if we want to do this particular reshuffling a bit later down the road (if this makes sense to you), but I think it will make this reducer easier to work with long term

consider if we need all these tests here. in general we're trying to follow the redux testing guidelines and avoid testing the redux internals as much as possible - preferring integrations tests with the UI. see https://redux.js.org/usage/writing-tests#guiding-principles

do we need a fresh getState or can we just use the one obtained at the top of this fn? do we expect it to change and if yes, could this create some inconsistencies in the behaviour?

we do not, good catch!

can we avoid the typecasting here? plus the same question on getState() use here

yeah I can remove the isRelevantPreviousState function

Hmmm, this is kinda weird now (or maybe I don't understand what this event is about): this will fire on every successful sign in attempt whereas my understanding is that this should fire instead when the tool card suggesting user to sign in is being displayed first which I think should be either near the code that triggers the tool call (if there is such a thing) or in the rendering method, just using the effect hooks correctly (meaning a clear component that shows sign in state once that can have a clear "onMount" effect setup)

hmm I thought this part of the code was only triggered when the login page was shown but possibly I'm mistaken (?)

You are calling track inside the signIn action which is triggered when user saw the tool card and decided to proceed with sign in and clicked the sign in button, but (as far as I understand from the event description) this event should be triggered when user just saw the tool card, before they decided to proceed:

This event is fired when the user is shown a prompt inviting them to sign in

oh yeah that's right... if the user do not proceed then the event wouldn't be tracked. Will fix it